-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Validation to BankAccount + CompanyStep + better handling for Plaid #3686
Conversation
One NABish thing I'm noticing is that hitting |
Not sure if this is intentional, but if the password is wrong and everything else is "correct", we just display an "Unknown error" growl. Might be a better UX to just say that the password is wrong. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work as always! I had a few suggestions, but no blockers (I guess except removing the TODO). Up to you if you want to act on any of the other suggestions.
reviewing in a minute! sorry, have been on a long candidate call this morning |
Thanks for the review @roryabraham I'm looking into these two now
|
@roryabraham can you clarify how you got to the "Unknown error"? I'm seeing this -> So I think maybe you are running into an unknown error 😅 |
Ready for another review. |
noticed a few things from the old e.com bank account setup: Doesn't look like we have phone number validation for the |
@nickmurray47 Mainly because they are also caught and thrown in PHP e.g. the phone is checked here
The placeholder is a great idea since it's not obvious what format we are preferring for that one so I'll add it now! Thanks!
The industry code selector is not in scope for this project, but there are plans to eventually migrate that over. |
sounds good! also, was running into the same error that @roryabraham pointed out. Not sure if you or Rory's already seen this but I think it's because we throw an error from Web-S if a user has too many errors in 24 hours, |
Hmm I'd actually expect us to be handling that here... I think we should maybe not block on this, but I'll create a follow up to verify that's all working correctly. |
Alright so I tried to look into this comment here -> #3686 (comment) And set that NVP + saw an error message. If that's your issue @nickmurray47 then you should be seeing this now: Can you verify with me whether you see that the next time you open the modal? |
Ok, so there's not a ton for me to go off of here... but I can suggest we try to debug (or just Are y'all using the recommended test credentials for setting up a bank account? @roryabraham any thoughts on this? This isn't happening for me so I'm inclined to move forward with this. |
Using the test credentials for the OPEN bank account. I also agree we can move forward with the PR and circle back to this error. |
Ok thanks, I just ran through again with that flow and didn't see any errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need to block on this – clearly it's an edge case and we don't need perfection right now.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging in version: 1.0.73-4🚀
|
🚀 Deployed to production in version: 1.0.74-0🚀
|
cc @roryabraham
Details
Adds validation to forms in the
BankAccount
/CompanyStep
Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/167761
Tests
QA Steps
No QA
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android